-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cli: debug zip uses read-only range probe #108313
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- just one nit re: an important comment that I think we should have.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @j82w, @joshimhoff, @nvanbenschoten, and @Santamaura)
pkg/cli/zip_table_registry.go
line 233 at r1 (raw file):
"crdb_internal.probe_ranges_1s_read_limit_100": { customQueryRedacted: `SELECT * FROM crdb_internal.probe_ranges(INTERVAL '1000ms', 'read') WHERE error != '' ORDER BY end_to_end_latency_ms DESC LIMIT 100;`, customQueryUnredacted: `SELECT * FROM crdb_internal.probe_ranges(INTERVAL '1000ms', 'read') WHERE error != '' ORDER BY end_to_end_latency_ms DESC LIMIT 100;`,
nit: is it worth putting a comment here noting the potential dangers of using write
here, so future maintainers have that context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks. I agree with @abarganier that we should add a comment about the corruption bug.
To reduce the chance of corruption issues when run against older clusters, the recently added use of 'crdb_internal.probe_ranges` is modified to use a `read` probe instead of a `write` probe. See discussion in cockroachdb#107720 Release note: None Epic: None
d89c92b
to
0f44cac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w, @nvanbenschoten, and @Santamaura)
pkg/cli/zip_table_registry.go
line 233 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: is it worth putting a comment here noting the potential dangers of using
write
here, so future maintainers have that context?
Done.
bors r=joshimhoff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thx for the fix! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @Santamaura)
Build failed (retrying...): |
Build succeeded: |
To reduce the chance of corruption issues when run against older clusters, the recently added use of
crdb_internal.probe_ranges
is modified to use aread
probe instead of awrite
probe.See discussion in #107720
Release note: None
Epic: None